Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Response encoding not stored in the ext context session #5543 #5544

Merged
merged 6 commits into from
Feb 1, 2025

Conversation

BalusC
Copy link
Contributor

@BalusC BalusC commented Jan 18, 2025

@BalusC BalusC requested review from arjantijms and mnriem January 18, 2025 14:02
@BalusC BalusC added this to the 4.0.10 milestone Jan 18, 2025
@@ -929,6 +929,11 @@ protected ResponseWriter createResponseWriter(FacesContext context) throws IOExc
// Save encoding in UIViewRoot for faster consult when Util#getResponseEncoding() is invoked again elsewhere.
context.getViewRoot().getAttributes().put(FACELETS_ENCODING_KEY, encoding);

// Save encoding in Session for consult in subsequent postback request as per spec section "2.5.2.2. Determining the Character Encoding".
Copy link
Contributor

@mnriem mnriem Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this stored in the session? Please review this with Servlet 6.0 specification, section 5.6. Internationalization in mind.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated in that comment, this is mandated by the JSF 2.3 specification chapter 2.5.2.2 Determining the Character Encoding, see:

"[P1-start-encoding]At the end of the render-response phase, the ViewHandler must store the response character encoding
used by the underlying response object (e.g., the servlet or portlet response) in the session (if and only if a session
already exists) under a well known, implementation-dependent key."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I stated before this needs to be reviewed with the specified Servlet 6.0 specification in mind. I do not object to setting it, but I do not see any logic that looks at the Servlet level. Note that JSF 2.3 is not supported and I assume you meant to refer to Faces specifications?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my fault, sorry, I meant Faces 4.0...

Anyhow, the spec has not changed there, see Jakarta Faces 4.0 specification.

As of that I don't see how this is related to the Servlet 6.0 specification, as the Faces specification already requires storing the character encoding in the session.

Copy link
Contributor Author

@BalusC BalusC Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this stored in the session?

Because last used form encoding is not sent back via headers/payload of a postback request per-se.

Please review this with Servlet 6.0 specification, section 5.6. Internationalization in mind.

It's indeed used for setRequestEncoding() as mentioned in that spec to remedy any breakages.

If the client hasn’t set character encoding and the request data is encoded with a different encoding than the default as described above, breakage can occur. To remedy this situation, setRequestCharacterEncoding(String enc) is available on ServletContext

@arjantijms arjantijms changed the title Fix #5543 Fix Response encoding not stored in the ext context session #5543 Jan 22, 2025
@bmaxwell
Copy link

@arjantijms , is this one ok to merge? We have some users needing the fix. Thanks !

@BalusC BalusC merged commit 60f58f7 into 4.0 Feb 1, 2025
3 checks passed
@BalusC BalusC deleted the issue_5543 branch February 1, 2025 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants